[VPEX][5c] local-env: --check cache purity, greenfield name, and databricks-connect insertion#5854
Conversation
Integration test reportCommit: 0eb1236
8 interesting tests: 4 SKIP, 2 RECOVERED, 2 flaky
Top 10 slowest tests (at least 2 minutes):
|
af9d41b to
c2e8b69
Compare
8b12b87 to
4716210
Compare
anton-107
left a comment
There was a problem hiding this comment.
Approving. Tight, correct follow-up: writeCache threaded through FetchConstraints with the pipeline passing !p.Check, and an existing cache is still read for offline fallback (reading isn't a mutation) — exactly right. Test pair is the right shape: unit (TestFetchConstraintsSkipsCacheWriteWhenDisabled) plus the strengthened end-to-end TestPipelineCheckMutatesNothing asserting the cache dir stays empty. All call sites updated. No findings.
4716210 to
d6462fd
Compare
d6462fd to
3127207
Compare
4bd0477 to
b09b9ce
Compare
|
codex review [P2] — duplicate databricks-connect pins — fixed in b09b9ce. Confirmed the bug (reproduced: 2 pins for both Fix: the multi-line branch now scans the whole array span ( |
A --check dry run must not mutate disk, but FetchConstraints wrote the fetched artifact into the cache dir on every successful live fetch — so a dry run still populated the cache whenever it was writable, even though the pipeline skips every other write-side step under --check. Thread a writeCache flag through FetchConstraints; the pipeline passes !p.Check. An existing cache is still read for offline fallback, since reading is not a mutation. Assert both directly (FetchConstraints with writeCache=false leaves the cache dir empty) and end-to-end (the --check pipeline test now checks the cache dir stays empty too). Reported by codex review of the pipeline PR. Co-authored-by: Isaac
b09b9ce to
ad1b317
Compare
ad1b317 to
e257732
Compare
|
codex review [P3] — trailing-comment mutation — fixed in e257732. Confirmed (reproduced: a |
Address two correctness nits from review of the pipeline PR (#5851). Since that PR was already queued to merge, the fixes land here in the follow-up. - mergeDatabricksConnect only ever *replaced* an existing databricks-connect element, so a default-mode sync of an existing project that didn't already pin databricks-connect wrote the file back with no pin; uv installed nothing and validate failed with E_VALIDATE. It now *inserts* the pin — into the dev array (single- or multi-line), creating the dev key and the [dependency-groups] table when absent — mirroring RenderFreshPyproject. Constraints-only (empty value) stays a no-op; the insert path is idempotent. - Greenfield rendered name = filepath.Base(ProjectDir), which is "." when run from the project's own directory (and "/" at root) — not a valid PEP 621 name, so uv sync rejected the file. projectName() now resolves via filepath.Abs and sanitizes to a valid PEP 508 identifier, falling back to a default when nothing usable remains. Co-authored-by: Isaac
e257732 to
0eb1236
Compare
|
isaac review — 2 MAJOR bugs in the databricks-connect insert path — both fixed in 0eb1236. Both reproduced and confirmed real:
All gates green (build, test, lint 0, deadcode, gofmt). |
Integration test reportCommit: 65dacaf
31 interesting tests: 17 flaky, 6 FAIL, 4 KNOWN, 2 RECOVERED, 2 SKIP
Top 50 slowest tests (at least 2 minutes):
|
…ks#5832) ## Why - The engine needs a real `PackageManager` implementation (uv) and a CLI entry point so a user can actually run the feature. - Wiring it in while keeping it `Hidden` lets the command be dogfooded and exercised by acceptance tests without becoming user-visible until the stack is complete. - This is the first layer reachable from `main`, so it also makes the whole `libs/localenv` package live for the `deadcode` checker. ## What - **`libs/localenv/uv.go`** — the uv implementation of `PackageManager`: discover/install uv, install the Python minor, `uv sync`, seed pip into the venv, validate; plus the `pip.conf` → `UV_INDEX_URL` bridge for Databricks-managed machines. - **`cmd/localenv/`** — the command tree matching `local-env python sync`: a `local-env` group (`Hidden: true`), a `python` subgroup, and the `sync` verb. Parent nodes use `root.ReportUnknownSubcommand`; `sync` uses `cobra.NoArgs`, resolves flags/bundle target, builds the `Pipeline` with the uv manager, and renders text or `--json`. All Cobra `Use` values + the `--json` command field come from the `libs/localenv` constants. - **`cmd/cmd.go`** — registers the group. ## Testing strategy - Unit tests for uv helper logic (discovery, `pip.conf` index-url, arg builders, stderr surfacing) (`uv_test.go`). - Runtime smoke test of the hidden 3-level command: absent from top-level help; help works at each level; unknown subcommand exits non-zero; bare group shows help; flags + mutual-exclusion + `NoArgs` behave. - Gates: `go build ./...`, `go test`, `golangci-lint`, `deadcode` (whole tree, no pragmas), `gofmt` — all green. - `cmd/localenv/` unit tests are intentionally deferred to the acceptance PR (databricks#5833), per the repo convention that user-visible CLI output is covered by acceptance tests. --- ## About this stack This is one of a series of small, stacked PRs that together add the `databricks local-env python sync` command — it provisions a local Python environment (Python version, `databricks-connect` pin, and dependency constraints) matched to a selected Databricks compute target. The work was split from one large branch into single-concern layers so each is independently reviewable; the command is kept hidden until the final PR so nothing is user-visible mid-stack. **Review bottom-up.** Each PR targets the previous one as its base branch (this PR targets `main`), so its diff shows only that layer. Layers 1–5 have merged; the original layer-5 PR (databricks#5828) was split into 5a/5b/5c during review. | # | PR | What | Status | |---|----|------|--------| | 1 | databricks#5823 | foundation: result types + env-key mapping | merged | | 2 | databricks#5824 | compute-target resolution | merged | | 3 | databricks#5826 | constraint fetch + offline cache | merged | | 4 | databricks#5827 | formatting-preserving pyproject.toml merge | merged | | 5a | databricks#5850 | package-manager interface + detection | merged | | 5b | databricks#5851 | six-phase pipeline orchestrator | merged | | 5c | databricks#5854 | --check cache purity, greenfield name, dbc insertion | merged | | 6 | **databricks#5832 ← you are here** | uv backend + CLI command (registered hidden) | | | 7 | databricks#5833 | acceptance tests | | | 8 | databricks#5835 | unveil (unhide + help + changelog) | | This pull request and its description were written by Isaac.
## Why - The command's user-visible behavior — text and `--json` output, and every error path — needs end-to-end coverage against the real CLI. - `cmd/localenv/` carries no unit tests by design, so acceptance tests are where that surface is verified (repo convention: user-visible CLI output is covered by acceptance tests). ## What - **`acceptance/localenv/`** — 9 scenarios driven through the (hidden) command against the in-process fake server: `help` (three-level tree), `no-target` (`E_NO_TARGET`), `flag-conflict` (Cobra mutual-exclusion), `manager-unsupported` (conda project → clean P1 exit), `env-unsupported` (404 → `E_ENV_UNSUPPORTED` at fetch), `json-error` (`--output json` error object), `serverless-check` (dry-run plan), `serverless-json` (`--json` plan), and `constraints-only`. - Scripts use `local-env python sync` and the `DATABRICKS_LOCALENV_CONSTRAINT_SOURCE` override; goldens show the `local-env python sync` command field and managed marker. No source changes. ## Testing strategy - Goldens generated with `-update` and verified **stable on a clean re-run** (no `-update`); all 9 subtests pass. - `musterr` guards the five expected-failure scenarios; `trace` shows the three output-producing ones. - Full acceptance suite run to confirm no regressions elsewhere (only pre-existing, environment-specific failures unrelated to this change). - Diff confined to `acceptance/localenv/`. - Independently verified by a review subagent (PASS — goldens, scripts, stubs, stale-refs, hygiene) and by codex (no issues). --- ## About this stack This is one of a series of small, stacked PRs that together add the `databricks local-env python sync` command — it provisions a local Python environment (Python version, `databricks-connect` pin, and dependency constraints) matched to a selected Databricks compute target. The work was split from one large branch into single-concern layers so each is independently reviewable; the command is kept hidden until the final PR so nothing is user-visible mid-stack. **Review bottom-up.** Layers 1–5 have merged (the original layer-5 PR databricks#5828 was split into 5a/5b/5c during review). | # | PR | What | Status | |---|----|------|--------| | 1 | databricks#5823 | foundation: result types + env-key mapping | merged | | 2 | databricks#5824 | compute-target resolution | merged | | 3 | databricks#5826 | constraint fetch + offline cache | merged | | 4 | databricks#5827 | formatting-preserving pyproject.toml merge | merged | | 5a | databricks#5850 | package-manager interface + detection | merged | | 5b | databricks#5851 | six-phase pipeline orchestrator | merged | | 5c | databricks#5854 | --check cache purity, greenfield name, dbc insertion | merged | | 6 | databricks#5832 | uv backend + CLI command (registered hidden) | | | 7 | **databricks#5833 ← you are here** | acceptance tests | | | 8 | databricks#5835 | unveil (unhide + help + changelog) | | This pull request and its description were written by Isaac.
Why
Follow-up fixes to the
local-env python syncpipeline (#5851, now merged), from codex and reviewer feedback. Three independent correctness issues in the merge / dry-run paths, none of which the pipeline PR's own tests caught:--checkstill wrote to disk.FetchConstraintspopulated the on-disk constraint cache on every successful live fetch, so a dry run mutated the cache directory even though the pipeline skips every other write-side step under--check.filepath.Base(ProjectDir)is.when run from the project's own directory (and/at root) — not a valid PEP 621/508 name, souv syncrejected the generatedpyproject.toml.pyproject.tomlwith a dev group but no databricks-connect was written back unchanged,uvinstalled nothing, and validation failed withE_VALIDATE. Greenfield worked only becauseRenderFreshPyprojectinserts the pin — a path the existing-project merge lacked.What
--checkno cache write — thread awriteCache boolthroughFetchConstraints; the pipeline passes!p.Check. An existing cache is still read for offline fallback (reading is not a mutation); only the write is suppressed under a dry run.projectName()resolves./""/root viafilepath.Absbefore taking the base and sanitizes to a valid PEP 508 identifier (non-alphanumeric runs collapse to-, trimmed), falling back toappwhen nothing usable remains.mergeDatabricksConnectnow inserts the pin into[dependency-groups].dev(single- or multi-line array), creating thedevkey and the[dependency-groups]table when absent, mirroringRenderFreshPyproject. Constraints-only (empty value) stays a no-op. Detection is comment-aware and spans the whole array, so a pin on thedev = [line or one carrying a trailing comment is rewritten in place rather than duplicated (a duplicate-pin bug codex caught in the first cut of this change). The insert path is idempotent.Testing strategy
--checkpurity:TestFetchConstraintsSkipsCacheWriteWhenDisabled, andTestPipelineCheckMutatesNothingstrengthened to assert the cache dir stays empty;TestPipelineCheckReRunPlanMatchesRealRunfor accurate re-run plans.TestProjectName,TestPipelineGreenfieldFromDotDirRendersValidName.TestMergeInserts{DatabricksConnectMultiLine,SingleLine,EmptyDev,DevKeyWhenAbsent,DependencyGroupsWhenAbsent}, no-duplicate regressionsTestMergeReplacesDatabricksConnect{OnDevLine,WithTrailingComment}, andTestMergeConstraintsOnlyDoesNotInsertDatabricksConnect.go build,go test,golangci-lint(0 issues),deadcode,gofmt— all green.About this stack
Follow-up to the
databricks local-env python syncstack; targetsmainnow that the pipeline PR (#5851) has merged.This pull request and its description were written by Isaac.